support hashable-1.4.3 #1717
support hashable-1.4.3 #1717
Conversation
| Left BadAdjacents -> do | ||
| putMVar cmv c | ||
| return False |
There was a problem hiding this comment.
Before the function would fail with an userError in this case. This change informs the caller when mining fails.
In practice there are tests where it can happen that the mining of a blocked chain is attempted. It was by pure chance that this didn't showed up before. The latest version of hashable changed the hash value of chain ids and thus the ordering of chains in tests, which caused this issue to surface.
There was a problem hiding this comment.
What do you think about applying a "if comments on github make it easy to understand the PR, they should be in the code" rule here?
There was a problem hiding this comment.
I am not sure in case of this comments, since they talk an old version of the code and the diff with the new version. I think, PR comments my be a better place for those.
But, generally, I agree, that comments that explain the behavior of the new code should be code comments.
| cid = unsafeChainId 9 | ||
| -- several tests in this file expect chain 9 |
There was a problem hiding this comment.
With the new hashable version the value of someChainId changes. This fixes the previous value.
| someChainIdAt v h = minimum $ chainIdsAt v h | ||
| -- guaranteed to succeed because the empty graph isn't a valid chain graph. |
There was a problem hiding this comment.
We could have fixed the value to value with the previous version of hashable, which would have avoided updating of golden files. But using the minimum seems like a more reasonable and intuitive default.
| @@ -344,7 +344,7 @@ constraints: any.Boolean ==0.2.4, | |||
| any.time-compat ==1.9.6.1, | |||
| time-compat -old-locale, | |||
| any.time-manager ==0.0.0, | |||
| any.tls ==1.7.0, | |||
| any.tls ==1.7.1, | |||
There was a problem hiding this comment.
this update is unrelated, but nice to have.
| Left BadAdjacents -> do | ||
| putMVar cmv c | ||
| return False |
There was a problem hiding this comment.
What do you think about applying a "if comments on github make it easy to understand the PR, they should be in the code" rule here?
The updates the golden files are caused to changes in the test harness and don't represent on-chain semantics.
Tested that the changes also work with hashable<1.4.3.